Sanitize MD and initiate addition of tests #43
Conversation
dependabot sends PRs with `[]()` in the title eg
[gh-actions](deps): Bump actions/setup-python from 4 to 5
making it not legitimate markdown
- [[gh-actions](deps): Bump actions/setup-python from 4 to 5](dandi/zarr_checksum#42) (253 days)
so with this change we just remove any of the [] symbols, making it a legit
- [gh-actions(deps): Bump actions/setup-python from 4 to 5](dandi/zarr_checksum#42) (253 days)
setup.cfg
Outdated
| test = | ||
| mypy | ||
| pytest | ||
| pytest-cov |
There was a problem hiding this comment.
This isn't used by the test environment.
There was a problem hiding this comment.
@yarikoptic pytest-cov is still unused.
Also, I would prefer it if, rather than defining a test extra, the test dependencies were instead listed in tox.ini.
There was a problem hiding this comment.
why (what advantage) to list in a tool specific tox.ini making it harder to setup environment for any other tool/direct invocation of pytest? I would really prefer to concentrate dependencies in a single location.
There was a problem hiding this comment.
I generally do not support any other tools than tox nor directly invoking pytest.
There was a problem hiding this comment.
I typically invoke pytest directly, and testing within debian and most often conda packages is done directly via pytest. it is odd to require some additional utility whenever pytest framework is already sufficient.
src/solidation/__main__.py
Outdated
| @@ -437,6 +437,11 @@ def ensure_aware(dt: datetime) -> datetime: | |||
| return dt.replace(tzinfo=timezone.utc) if dt.tzinfo is None else dt | |||
|
|
|||
|
|
|||
| def sanitize_md(s: str) -> str: | |||
| # Remove `[]` symbols to ensure correct markdown in the references | |||
| return re.sub(r'[\[\]]', '', s) | |||
There was a problem hiding this comment.
Wouldn't it be better to instead escape brackets, changing [ to \[?
There was a problem hiding this comment.
dang, I didn't even try but it indeed seems to work:
so I guess we would also need to escape the \:
ok, will do... now
src/solidation/tests/test_main.py
Outdated
| @@ -0,0 +1,5 @@ | |||
| from ..__main__ import sanitize_md | |||
There was a problem hiding this comment.
Move this file to tests/test_main.py relative to the project root (i.e., outside of src/).
There was a problem hiding this comment.
I really see little benefits and only inconveniences from such separation, but ok, will do, since you are the lead on this one.
There was a problem hiding this comment.
done in 4978b8a which also required replicating listing of tests now for all other tox components which take care about source code
tox.ini
Outdated
| [testenv] | ||
| extras = test | ||
| commands = | ||
| python -m pytest -v {posargs} src |
There was a problem hiding this comment.
Change src to tests after moving the test file.
| @@ -0,0 +1,27 @@ | |||
| name: Unit Test | |||
There was a problem hiding this comment.
| name: Unit Test | |
| name: Test |
There was a problem hiding this comment.
Made it such since we also have Smoke Test. Makes it more explicit on what is the difference between the two.
tests/__init__.py
Outdated
There was a problem hiding this comment.
This file is unnecessary and should be removed.
setup.cfg
Outdated
| test = | ||
| mypy | ||
| pytest | ||
| pytest-cov |
There was a problem hiding this comment.
@yarikoptic pytest-cov is still unused.
Also, I would prefer it if, rather than defining a test extra, the test dependencies were instead listed in tox.ini.
Co-authored-by: John T. Wodder II <[email protected]>
John: This has nothing to do with whether imports are sorted in the tests. src_paths = tests declares that any modules immediately within tests/ are to be treated as first-party packages for sorting purposes, but we shouldn't be importing anything from modules within tests/ anyway.
=== Do not change lines below ===
{
"chain": [],
"cmd": "pre-commit autoupdate",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
…test also removed unnecessary now dev installation of joblib # extras test vs tox.ini: Most recent debate on tox.ini vs extra_depends is at con/solidation#43 (comment) Although I appreciate the convenience of "tox" as an entry point for testing, I increasingly find no support for it to be "the" location for testing depends listing. Moreover I keep running into cases of needing to fish out test dependencies somewhere else (tox.ini, nox etc) which then can differ across projects, and also require me to adopt some avoidable runtime etc overhead from running those extra test shims whenever pytest is just good enough. My arguments for generally adopting an approach of specifying test dependencies in setup.{cfg,py} or other "generic" locations as optional are: - pytest, and its extentions are used (imported) inside the tests. I, in 99% of cases, do consider "tests" to be the part of the source code. I would not consider them part of the source code whenever there is an outside test battery which is developed/maintained independently of the source code. As such, I think that dependencies for tests, as part of the source code, should be listed alongside with dependencies for the build/installation/run time dependencies. Some distributions even do "import test" across entire source code distribution and thus tend to decide or to request exclusion from source distributions (IMHO the wrong step in most of the cases). Ref: dandi/dandi-schema#249 - It is unfortunate that there is no "standard" convention on how/where to specify such test requirements, so I think it is ok to adopt [test] as the general convention among our projects. - tox.ini looses NOTHING from using "extras". - tox.ini is the correct location to describe environments and dependncies for external to source code tools/modules, i.e those not imported explicitly anywhere in the source code. - with description of test dependencies alongside with the main dependencies would benefit downstream distribution (debian, conda, gentoo, etc) packagers since they do not need to fish around for what other dependencies to install/recommend/suggest for the package. - I do appreciate the fact that test dependencies alone are not sufficient to run the tests, but invocation of the pytest is standardized enough to just invoke it against the source code. (given dependencies are installed) That is e.g. how dh-python helper in Debian operates -- if pytest dependency announced, just run pytest automagically. # joblib: joblib dev listing was added in commit 6375816 Author: John T. Wodder II <[email protected]> Date: Fri Mar 19 08:42:47 2021 -0400 likely to use ❯ git describe --contains 457d2c8a926105fd156b1dfccfaae3e500d22451 1.1.0~13 ❯ git show 457d2c8a926105fd156b1dfccfaae3e500d22451 commit 457d2c8a926105fd156b1dfccfaae3e500d22451 Author: John T. Wodder II <[email protected]> Date: Thu Mar 18 06:07:06 2021 -0400 Use inspect.signature() instead of inspect.getfullargspec() (#1165) This fixes a bug with the ignore parameter when the cached function is decorated Co-authored-by: Loïc Estève <[email protected]> and we have already joblib ~= 1.1 so should be all set.
…test Most recent debate on tox.ini vs extra_depends is at con/solidation#43 (comment) Although I appreciate the convenience of "tox" as an entry point for testing, I increasingly find no support for it to be "the" location for testing depends listing. Moreover I keep running into cases of needing to fish out test dependencies somewhere else (tox.ini, nox etc) which then can differ across projects, and also require me to adopt some avoidable runtime etc overhead from running those extra test shims whenever pytest is just good enough. My arguments for generally adopting an approach of specifying test dependencies in setup.{cfg,py} or other "generic" locations as optional are: - pytest, and its extentions are used (imported) inside the tests. I, in 99% of cases, do consider "tests" to be the part of the source code. I would not consider them part of the source code whenever there is an outside test battery which is developed/maintained independently of the source code. As such, I think that dependencies for tests, as part of the source code, should be listed alongside with dependencies for the build/installation/run time dependencies. Some distributions even do "import test" across entire source code distribution and thus tend to decide or to request exclusion from source distributions (IMHO the wrong step in most of the cases). Ref: dandi/dandi-schema#249 - It is unfortunate that there is no "standard" convention on how/where to specify such test requirements, so I think it is ok to adopt [test] as the general convention among our projects. - tox.ini looses NOTHING from using "extras". - tox.ini is the correct location to describe environments and dependncies for external to source code tools/modules, i.e those not imported explicitly anywhere in the source code. - with description of test dependencies alongside with the main dependencies would benefit downstream distribution (debian, conda, gentoo, etc) packagers since they do not need to fish around for what other dependencies to install/recommend/suggest for the package. - I do appreciate the fact that test dependencies alone are not sufficient to run the tests, but invocation of the pytest is standardized enough to just invoke it against the source code. (given dependencies are installed) That is e.g. how dh-python helper in Debian operates -- if pytest dependency announced, just run pytest automagically.
dependabot sends PRs with
[]()in the title egmaking it not legitimate markdown
so with this change we just remove any of the [] symbols, making it a legit